fix(hindsight): pass retain_async=True to aretain_batch in tool handler by yingliang-zhang · Pull Request #60648 · NousResearch/hermes-agent

yingliang-zhang · 2026-07-08T02:51:55Z

Problem

The hindsight_retain tool handler calls client.aretain_batch() without retain_async, defaulting to synchronous mode. On banks with significant data, the LLM fact-extraction call can take 60–600+ seconds, far exceeding the 120s _DEFAULT_TIMEOUT. This produces an opaque TimeoutError with an empty message: "Failed to store memory: ".

The auto-retain path (sync_turn, ~L1685) correctly passes retain_async=retain_async_flag (True) to aretain_batch. The tool call path (~L1721) does not, despite the comment at ~L1715 stating "aretain_batch takes bank_id/retain_async as call args, not item keys".

Fix

Add retain_async=True to the aretain_batch call in the tool handler, consistent with the auto-retain path. The tool now returns immediately after the server accepts the request, matching sync_turn behavior.

Why not #37838?

PR #37838 adds retain_async=True to _build_retain_kwargs() instead. However, this is ineffective because item.pop("retain_async", None) at ~L1717 immediately removes it from the item dict before aretain_batch is called — the value never reaches the server.

This PR passes retain_async=True directly to aretain_batch as a call argument, which is the correct approach.

Closes #29079 (Embedded Hindsight retain reports failure while async retain later appears in recall).
Related: #14950 (per-operation timeout), #42466 (cron retain race).
Supersedes approach in #37838.

alt-glitch · 2026-07-08T03:07:46Z

This was generated by AI during triage.

Duplicate of #22836 (earliest open, canonical). Both route hindsight_retain through client.aretain_batch(bank_id=..., items=[...], retain_async=True) - the same fire-and-forget mechanism at the same call site. #44414 was already marked a duplicate of #22836 for the same reason. Related: #37838 (competing _build_retain_kwargs approach), #29079 (the bug this closes). A maintainer can pick between #22836 and #60648.

yingliang-zhang · 2026-07-08T04:55:54Z

Thanks for the triage @alt-glitch. I've analyzed all three competing PRs and want to clarify the differences:

#22836 (by @spirotot, May 9) — The earliest PR. It targets an older version of handle_tool_call that used client.aretain(**retain_kwargs). It refactors the call to client.aretain_batch(bank_id=..., items=[...], retain_async=True), pops bank_id/retain_async from kwargs, and changes the success message. Currently has merge conflicts (mergeable_state=dirty) because the upstream code has since been refactored — the tool handler already uses aretain_batch now.

#60648 (this PR, Jul 8) — Minimal fix against current main. The tool handler already calls client.aretain_batch(bank_id=..., items=[item]) — this PR simply adds retain_async=True to that existing call (+4/-2 lines). No refactoring needed because upstream already did the aretain → aretain_batch migration.

#37838 (by @kkangg1, Jul 8) — Adds retain_async=True to _build_retain_kwargs(). However, as documented in this PR's description, this is ineffective: the item dict gets retain_async popped via item.pop("retain_async", None) before being passed to aretain_batch, so the flag never reaches the client call.

Summary: #60648 is the correct minimal fix for current main. #22836 was the canonical fix but its refactoring has been partially superseded by upstream changes and now conflicts. #37838 doesn't actually work due to the pop. I'll close #37838 as superseded by this PR. A maintainer can pick between #22836 (needs rebase) and #60648 (clean, minimal).

teknium1

Thanks for targeting the current aretain_batch call site; the missing call-level flag is present on current main at plugins/memory/hindsight/__init__.py:1721.

Problems

retain_async=True hard-codes behavior that is configurable today. The provider loads self._retain_async from config at plugins/memory/hindsight/__init__.py:1354, and sync_turn() forwards that value to aretain_batch() at plugins/memory/hindsight/__init__.py:1681-1686. A user with retain_async: false would have tool retains behave differently from auto-retains.
The changed behavior lacks a tool-handler regression test. tests/plugins/memory/test_hindsight_provider.py:641-653 exercises this path but does not assert the call-level flag; the existing sync_turn test demonstrates both configuration values are meaningful at tests/plugins/memory/test_hindsight_provider.py:951-962.

Suggested changes

Pass a captured self._retain_async value as the call-level argument instead of a literal.
Add default-true and configured-false assertions for hindsight_retain.

Automated hermes-sweeper review.

yingliang-zhang · 2026-07-11T12:51:51Z

Thanks for the review @teknium1. Addressed the feedback: retain_async=True is now replaced with self._retain_async (read from provider config at init, same pattern as sync_turn at line 1664). This ensures users who set retain_async: false in config.yaml get consistent behavior across both code paths.

Pushed to the PR branch.

yingliang-zhang · 2026-07-31T06:22:56Z

Thanks for the triage note. We're keeping this PR open.

While #22836 targets the same aretain_batch call site, this PR's fix scope is narrower and the implementation diverges in approach. Both PRs can coexist — if the maintainers prefer to merge #22836 first, we'll rebase onto that and drop any overlapping changes. For now, keeping both open gives the maintainers flexibility on which approach to adopt.

…rd-coded True Sweeper feedback on NousResearch#60648: hard-coding retain_async=True ignores the user's retain_async config setting. Read self._retain_async (loaded from config.yaml at init) for consistency with the sync_turn path, which already uses the same pattern at line 1664.

Port of NousResearch#60648 onto post-NousResearch#102117 main. The refactor rewrote the provider file (2440→1214 lines, machinery decomposed to plugins/memory/hindsight/{embedded,settings,setup}.py) and introduced a _retain_batch helper that already accepts retain_async — but the hindsight_retain tool handler calls it without forwarding the configured self._retain_async, so the tool path silently dropped the async/sync choice (aretain_batch fell back to its own server default). Forward the configured mode explicitly. Test: parametrized True/False forwarding assertion added to TestToolHandlers (same as the original PR).

yingliang-zhang · 2026-09-04T15:55:35Z

Rebased onto current main (d3630f8532, post-#102117 refactor) — ported: the provider file was rewritten (machinery decomposed to hindsight/{embedded,settings,setup}.py) and the retain dispatch became the _retain_batch helper which already accepts retain_async; the tool handler now forwards the configured self._retain_async instead of dropping it. Same parametrized forwarding test as the original PR.

yingliang-zhang · 2026-09-21T08:42:44Z

Good catch — fixed. The tool handler now forwards the configured mode instead of the literal:

self._retain_batch(item, bank_id=self._bank_id, retain_async=self._retain_async)

A user with retain_async: false now gets identical semantics for tool-initiated and auto retains. Added test_tool_retain_forwards_configured_retain_async (parametrized over both config values) asserting the call-level flag matches the provider config — it fails on the pre-fix head.

teknium1 · 2026-09-23T09:51:47Z

Closing: the bundled Hindsight provider this PR patches has moved out of this repo.

Thanks @yingliang-zhang for this contribution. In #119888 (merge 9d799e0531c; removal commit 4cbf862abe4) the in-tree plugins/memory/hindsight/ provider was removed — Hindsight now installs from the plugin catalog and its code lives in vectorize-io/hindsight hindsight-integrations/hermes (maintained by @nicoloboschi). There is no longer any code in this repo for the Hindsight half of this PR to patch, so we are closing every open PR against the bundled provider rather than leaving them stranded.

Triage notes:

Final revision forwards self._retain_async from the hindsight_retain tool handler into _retain_batch, matching the auto-retain path (competing PRs fix(hindsight): make hindsight_retain tool fire-and-forget #22836/fix(hindsight): pass retain_async=True in hindsight_retain tool handler #37838/fix(hindsight): make tool-call retain async to match auto-retain behavior #44414 noted in-thread were never merged either). Bundled provider removed in hindsight moves to the plugin catalog (auto-migrated on first start / hermes update) #119888 (9d799e0); at the catalog pin init.py:1437 still calls self._retain_batch(item, bank_id=self._bank_id) while the auto path at :1352 passes retain_async, so the inconsistency is still present upstream.
Still relevant at the catalog pin (dc750388)? Yes — the same code is at hindsight-integrations/hermes/__init__.py:1437 in the upstream tree. It is listed with your credit in Fixes from Hermes-side PRs worth carrying into hindsight-integrations/hermes vectorize-io/hindsight#4662 so it is not lost; if you want to carry the fix yourself, please open it against vectorize-io/hindsight — it would be welcome there.

If you believe this was closed in error, comment and we will reopen.

(Bulk-closed in the hindsight-move close pass.)

The hindsight_retain tool handler called _retain_batch without forwarding the configured self._retain_async, so tool retains silently dropped the async/sync choice and aretain_batch fell back to its own server default. retain_async is a call-level arg (never an item key), so the tool handler must pass it explicitly. Forward the configured mode, matching the auto-retain path. Ported from NousResearch/hermes-agent#60648 (closed in the hindsight-move close pass). Tracked in vectorize-io#4662.

yingliang-zhang mentioned this pull request Jul 8, 2026

fix(hindsight): pass retain_async=True in hindsight_retain tool handler #37838

Closed

6 tasks done

alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jul 8, 2026

This was referenced Jul 9, 2026

fix(hindsight): queue async retain tool calls #61513

Closed

fix(hindsight): pass retain_async from retain tool #61512

Closed

teknium1 reviewed Jul 10, 2026

View reviewed changes

Comment thread plugins/memory/hindsight/__init__.py Outdated

teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 10, 2026

yingliang-zhang force-pushed the fix/hindsight-retain-async-tool-handler branch 2 times, most recently from 6f5edaf to ec5c54a Compare July 11, 2026 12:51

yingliang-zhang force-pushed the fix/hindsight-retain-async-tool-handler branch from ec5c54a to ee05d63 Compare July 11, 2026 12:52

yingliang-zhang force-pushed the fix/hindsight-retain-async-tool-handler branch from ee05d63 to fc066b3 Compare July 11, 2026 14:02

This was referenced Jul 19, 2026

Feature Request: Per-operation timeout configuration for Hindsight memory provider #14950

Open

Embedded Hindsight retain reports failure while async retain later appears in recall #29079

Open

teknium1 added the area/memory Memory subsystem: store, providers, sync, background reviews label Jul 19, 2026

yingliang-zhang force-pushed the fix/hindsight-retain-async-tool-handler branch from 9508bb6 to 04a40fc Compare July 30, 2026 05:54

yingliang-zhang force-pushed the fix/hindsight-retain-async-tool-handler branch from 04a40fc to 5f06cd0 Compare August 20, 2026 23:11

yingliang-zhang force-pushed the fix/hindsight-retain-async-tool-handler branch from 5f06cd0 to 0976bc8 Compare August 26, 2026 10:05

yingliang-zhang force-pushed the fix/hindsight-retain-async-tool-handler branch from 0976bc8 to 65af769 Compare September 4, 2026 15:55

kvnloo mentioned this pull request Sep 13, 2026

Label audit (unverified): open tickets tagged duplicate or invalid #109552

Closed

teknium1 mentioned this pull request Sep 23, 2026

Fixes from Hermes-side PRs worth carrying into hindsight-integrations/hermes vectorize-io/hindsight#4662

Open

teknium1 closed this Sep 23, 2026

yingliang-zhang mentioned this pull request Sep 23, 2026

fix(hermes): pass retain_async=True to aretain_batch in tool handler vectorize-io/hindsight#4671

Open

Navigation Menu

fix(hindsight): pass retain_async=True to aretain_batch in tool handler - #60648

fix(hindsight): pass retain_async=True to aretain_batch in tool handler#60648
yingliang-zhang wants to merge 1 commit into
NousResearch:mainfrom
yingliang-zhang:fix/hindsight-retain-async-tool-handler

yingliang-zhang commented Jul 8, 2026

Uh oh!

alt-glitch commented Jul 8, 2026

Uh oh!

yingliang-zhang commented Jul 8, 2026

Uh oh!

teknium1 left a comment

Uh oh!

Uh oh!

yingliang-zhang commented Jul 11, 2026

Uh oh!

yingliang-zhang commented Jul 31, 2026

Uh oh!

yingliang-zhang commented Sep 4, 2026

Uh oh!

yingliang-zhang commented Sep 21, 2026

Uh oh!

teknium1 commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

yingliang-zhang commented Jul 8, 2026

Problem

Fix

Why not #37838?

Uh oh!

alt-glitch commented Jul 8, 2026

Uh oh!

yingliang-zhang commented Jul 8, 2026

Uh oh!

teknium1 left a comment

Choose a reason for hiding this comment

Problems

Suggested changes

Uh oh!

Uh oh!

yingliang-zhang commented Jul 11, 2026

Uh oh!

yingliang-zhang commented Jul 31, 2026

Uh oh!

yingliang-zhang commented Sep 4, 2026

Uh oh!

yingliang-zhang commented Sep 21, 2026

Uh oh!

teknium1 commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants